Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: add import support to ssrModuleLoader #5197

Merged
merged 22 commits into from
Oct 27, 2021
Merged

fix: add import support to ssrModuleLoader #5197

merged 22 commits into from
Oct 27, 2021

Conversation

natemoo-re
Copy link
Contributor

@natemoo-re natemoo-re commented Oct 4, 2021 •
edited
Loading

Description

This PR adds SSR support for loading Node ESM packages using import rather than always using require. This improves compatibility with the growing number of packages that are shipped as ESM only.

Additional context

This is implemented using an approach that Snowpack has been using with great success.

The basic steps are:
1. Attempt to require the package
2. Handle any errors that may have happened during the require
- If it's a SyntaxError and we see an import or export keyword, the user is likely using an outdated Node version. Give them a helpful error.
- If it's the known ERR_REQUIRE_ESM error, attempt to import the module.
- If it's any other error, surface it with another throw.

This PR has been updated to load everything (CJS and ESM) via import() rather than require with an import() fallback.

One caveat to this PR: TypeScript currently doesn't have a way to not compile dynamic import(), so we need to use the native-import.js file to load it. When typescript@4.5.0 is out of Beta and is officially released, this can be cleaned up significantly.

What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the Commit Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@Shinigami92 Shinigami92 self-requested a review October 5, 2021 04:18
@FredKSchott
Copy link
Contributor

Just wanted to chime in to give a +1 to this approach as Snowpack creator/maintainer: we landed this support in Snowpack several months ago and have had no issues that I'm aware of.

@patak-dev
Copy link
Member

Thanks for this PR @natemoo-re, we'll discuss it in this week's team meeting.

@benmccann
Copy link
Collaborator

Just leaving a note here to reflect discussion from Discord

If it's a SyntaxError and we see an import or export keyword, the user is likely using an outdated Node version. Give them a helpful error.
If it's the known ERR_REQUIRE_ESM error, attempt to import the module.

The package.json specifies engines, so we know the user can't be on a version of Node that doesn't support import. Thus, I would use import only and not try require first

@benmccann benmccann added feat: ssr p4-important Violate documented behavior or significantly improves performance (priority) labels Oct 5, 2021
@aleclarson aleclarson self-requested a review October 5, 2021 16:35
aleclarson
aleclarson suggested changes Oct 5, 2021
View reviewed changes
packages/vite/native-import.js Outdated Show resolved Hide resolved
packages/vite/src/node/ssr/ssrModuleLoader.ts Outdated Show resolved Hide resolved
packages/vite/src/node/ssr/ssrModuleLoader.ts Outdated Show resolved Hide resolved
packages/vite/src/node/ssr/ssrModuleLoader.ts Outdated Show resolved Hide resolved
packages/vite/src/node/ssr/ssrModuleLoader.ts Outdated Show resolved Hide resolved
packages/vite/src/node/ssr/ssrModuleLoader.ts Outdated Show resolved Hide resolved
@natemoo-re natemoo-re changed the title Add import support to ssrModuleLoader fix: add import support to ssrModuleLoader Oct 5, 2021
@natemoo-re natemoo-re marked this pull request as ready for review October 5, 2021 19:32
natemoo-re
natemoo-re commented Oct 5, 2021
View reviewed changes
packages/vite/src/node/ssr/ssrModuleLoader.ts Outdated Show resolved Hide resolved
packages/vite/src/node/ssr/ssrModuleLoader.ts Outdated Show resolved Hide resolved
packages/vite/src/node/ssr/ssrModuleLoader.ts Outdated Show resolved Hide resolved
@natemoo-re
Copy link
Contributor Author

Updated to use import and now tests are failing with a Segmentation fault... not sure what's going on there.

@natemoo-re natemoo-re mentioned this pull request Oct 5, 2021
Merged
80 tasks
aleclarson
aleclarson previously approved these changes Oct 5, 2021
View reviewed changes
@natemoo-re
Copy link
Contributor Author

natemoo-re commented Oct 6, 2021 •
edited
Loading

Ah, just figured out why the test suite is failing! Will take some time to see if I can get things running again locally.

Jest ships with experimental support for ECMAScript Modules (ESM).
https://jestjs.io/docs/ecmascript-modules

Edit Welp, --experimental-vm-modules didn't seem to fix it. Any assistance on this would be greatly appreciated!

@SamVerschueren SamVerschueren mentioned this pull request Oct 7, 2021
Merged
9 tasks
@SamVerschueren
Copy link
Contributor

@natemoo-re I also ran into the segfault issue with Jest. See #5213 (comment). I wasn't able to solve it as I think it's something with Node or Jest that causes this...

@Shinigami92 Shinigami92 marked this pull request as draft October 7, 2021 10:15
aleclarson
aleclarson previously approved these changes Oct 12, 2021
View reviewed changes
@natemoo-re natemoo-re marked this pull request as ready for review October 15, 2021 19:20
@matthewp
Copy link
Contributor

Here's a PR with Astro's test suite passing with this change: withastro/astro#1585

@bluwy bluwy mentioned this pull request Oct 19, 2021
Merged
@bluwy
Copy link
Member

bluwy commented Oct 19, 2021

I've also tested this branch locally at sveltejs/vite-plugin-svelte#204, and the changes here are working as well.

This was referenced Oct 21, 2021
Closed
Closed
@patak-dev patak-dev added this to the 2.7 milestone Oct 22, 2021
patak-dev
patak-dev approved these changes Oct 27, 2021
View reviewed changes
Copy link
Member

@patak-dev patak-dev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for testing this in Astro and Svelte @matthewp @bluwy. We discussed with @sodatea, and we think that with these external tests available it is safe to merge this PR as part of the next minor as we will also get extra testing from the beta period. Merging this one now in preparations for this release

@patak-dev patak-dev merged commit baba1f9 into vitejs:main Oct 27, 2021
@haoqunjiang haoqunjiang mentioned this pull request Nov 10, 2021
Merged
9 tasks
@aleclarson aleclarson mentioned this pull request Nov 12, 2021
Merged
9 tasks
@benmccann
Copy link
Collaborator

It looks the segfault when running tests with Jest is caused by a bug in v8. That issue can't be immediately resolved as a blocker must be resolved first.

@swyxio swyxio mentioned this pull request Nov 28, 2021
Open
This was referenced Nov 29, 2021
Closed
Closed
@korywka korywka mentioned this pull request Dec 3, 2021
Closed
@Jakeii Jakeii mentioned this pull request Jan 8, 2022
Closed
@ZYinMD ZYinMD mentioned this pull request Apr 26, 2022
Closed
7 tasks
@bluwy bluwy mentioned this pull request Oct 5, 2023
Closed
9 tasks
@sapphi-red sapphi-red mentioned this pull request Oct 9, 2023
Merged
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@drwpow drwpow drwpow left review comments

@benmccann benmccann benmccann left review comments

@patak-dev patak-dev patak-dev approved these changes

@aleclarson aleclarson aleclarson left review comments

@Shinigami92 Shinigami92 Awaiting requested review from Shinigami92

Assignees
No one assigned
Labels
feat: ssr p4-important Violate documented behavior or significantly improves performance (priority)
Projects
None yet
Milestone
2.7
Development

Successfully merging this pull request may close these issues.